Implement iterator specialization traits on more adapters #85528

the8472 · 2021-05-20T20:58:07Z

This adds

TrustedLen to Skip and StepBy
TrustedRandomAccess to Skip
InPlaceIterable and SourceIter to Copied and Cloned

The first two might improve performance in the compiler itself since skip is used in several places. Constellations that would exercise the last point are probably rare since it would require an owning iterator that has references as Items somewhere in its iterator pipeline.

Improvements for Skip:

# old
test iter::bench_skip_trusted_random_access                     ... bench:       8,335 ns/iter (+/- 90)

# new
test iter::bench_skip_trusted_random_access                     ... bench:       2,753 ns/iter (+/- 27)

rust-highfive · 2021-05-20T20:58:10Z

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

joshtriplett · 2021-05-20T22:10:28Z

@bors try @rust-timer queue

rust-timer · 2021-05-20T22:10:29Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-05-20T22:10:51Z

⌛ Trying commit 838b07beaca15f5614619bcee070ef5263a805ed with merge e056521d347c0ec9e2f6a0a140406f4b0a06c011...

bors · 2021-05-20T23:00:32Z

☀️ Try build successful - checks-actions
Build commit: e056521d347c0ec9e2f6a0a140406f4b0a06c011 (e056521d347c0ec9e2f6a0a140406f4b0a06c011)

rust-timer · 2021-05-20T23:00:34Z

Queued e056521d347c0ec9e2f6a0a140406f4b0a06c011 with parent 99e3aef, future comparison URL.

rust-timer · 2021-05-21T02:53:23Z

Finished benchmarking try commit (e056521d347c0ec9e2f6a0a140406f4b0a06c011): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

the8472 · 2021-05-21T12:59:26Z

Looks like noise. Or if it isn't then it's tiny impact at least. I guess the skip uses in the compiler aren't critical then.

I'll write some benchmarks to see if these actually improve some assembly.

library/core/src/iter/adapters/cloned.rs

library/core/src/iter/adapters/skip.rs

the8472 · 2021-06-12T09:18:17Z

I have added a benchmark

# old
test iter::bench_skip_trusted_random_access                     ... bench:       8,335 ns/iter (+/- 90)

# new
test iter::bench_skip_trusted_random_access                     ... bench:       2,753 ns/iter (+/- 27)

the8472 · 2021-06-12T09:27:06Z

library/core/src/iter/adapters/skip.rs

+            if Self::MAY_HAVE_SIDE_EFFECT && idx == 0 {
+                for skipped_idx in 0..self.n {
+                    drop(try_get_unchecked(&mut self.iter, skipped_idx));
+                }


In light of #85969 I must note that this alters (undocumented) side-effects for backwards iteration, albeit in less surprising ways.

This block is written to preserve side-effects of the skipped portion of the iterator during forward iteration. During backward iteration it will lead to the whole skipped portion being drained on the last (backwards) step instead of only the first item beyond the skipped portion, i.e. it behaves as if the last item were obtained by a next() instead of next_back().

Hm, so, my interpretation of what you're saying: we're picking idx == 0, which might not actually be the first index the user iterates over (and won't be in backwards iteration).

But I think my interpretation is probably wrong -- I've tried several times and I cannot manage to find a case where your patch causes different behavior in map(|i| dbg!(i)).skip(n) behavior in forward or backward iteration. Do you have an example where it makes causes an observable difference?

next_back doesn't touch the inner's skipped prefix. But anything that goes backwards via __iterator_get_unchecked (e.g. Zip) would once it hits index 0.

@the8472 Okay, this is a slight behavioral change to behavior we never made a promise about. I think it's a change we're allowed to make, but to be safe, that decision should be left to t-libs-api FCP.

Do you mind writing up the details about before/after differences so that that can be done?

yaahc · 2021-06-21T22:13:26Z

Going to pass this one off because I'm not familiar enough with this part of std to approve this by myself.

r? @m-ou-se

bors · 2021-07-29T02:58:58Z

☔ The latest upstream changes (presumably #85874) made this pull request unmergeable. Please resolve the merge conflicts.

rfcbot · 2023-10-08T03:55:33Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

the8472 · 2024-01-10T18:11:11Z

FCP complete, this should be ready to merge. ping @Amanieu or reassign to a libs reviewer now that the libs-api question is settled.

dtolnay

Thank you! Nice work.

dtolnay · 2024-01-21T02:38:25Z

@bors r+

bors · 2024-01-21T02:38:27Z

📌 Commit 37d26c7 has been approved by dtolnay

It is now in the queue for this repository.

bors · 2024-01-21T03:50:18Z

⌛ Testing commit 37d26c7 with merge 7328588...

Implement iterator specialization traits on more adapters This adds * `TrustedLen` to `Skip` and `StepBy` * `TrustedRandomAccess` to `Skip` * `InPlaceIterable` and `SourceIter` to `Copied` and `Cloned` The first two might improve performance in the compiler itself since `skip` is used in several places. Constellations that would exercise the last point are probably rare since it would require an owning iterator that has references as Items somewhere in its iterator pipeline. Improvements for `Skip`: ``` # old test iter::bench_skip_trusted_random_access ... bench: 8,335 ns/iter (+/- 90) # new test iter::bench_skip_trusted_random_access ... bench: 2,753 ns/iter (+/- 27) ```

bors · 2024-01-21T08:05:18Z

💥 Test timed out

rust-log-analyzer · 2024-01-21T09:19:34Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

the8472 · 2024-01-21T09:23:36Z

Apple runner hanging.

@bors retry

bors · 2024-01-21T11:17:49Z

⌛ Testing commit 37d26c7 with merge fa40433...

bors · 2024-01-21T13:15:29Z

☀️ Test successful - checks-actions
Approved by: dtolnay
Pushing fa40433 to master...

rust-timer · 2024-01-21T15:11:53Z

Finished benchmarking commit (fa40433): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-1.0%	[-1.5%, -0.5%]	2
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 662.48s -> 663.269s (0.12%)
Artifact size: 308.40 MiB -> 308.35 MiB (-0.02%)

rust-highfive assigned yaahc May 20, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2021

the8472 force-pushed the iter-markers branch from ae55176 to 838b07b Compare May 20, 2021 21:01

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 20, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 21, 2021

yaahc reviewed Jun 3, 2021

View reviewed changes

library/core/src/iter/adapters/cloned.rs Outdated Show resolved Hide resolved

library/core/src/iter/adapters/skip.rs Outdated Show resolved Hide resolved

the8472 force-pushed the iter-markers branch 2 times, most recently from 3b3e1bb to 5e1a4a3 Compare June 12, 2021 09:12

the8472 commented Jun 12, 2021

View reviewed changes

the8472 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 19, 2021

rust-highfive assigned m-ou-se and unassigned yaahc Jun 21, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2021

the8472 force-pushed the iter-markers branch from 5e1a4a3 to 634a9b0 Compare July 29, 2021 22:44

This comment has been minimized.

Sign in to view

apiraino removed the to-announce Announce this issue on triage meeting label Oct 12, 2023

the8472 added 4 commits January 10, 2024 18:55

implement TrustedLen for StepBy

a2a7caa

implement TrustedRandomAccess and TrustedLen for Skip

451a3b1

bench trustedrandomaccess specialization in zip

3aa7313

Implement in-place iteratation markers for iter::{Copied, Cloned}

37d26c7

the8472 force-pushed the iter-markers branch from 3af552a to 37d26c7 Compare January 10, 2024 18:08

dtolnay assigned dtolnay and unassigned Amanieu Jan 21, 2024

dtolnay approved these changes Jan 21, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2024

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 21, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 21, 2024

bors merged commit fa40433 into rust-lang:master Jan 21, 2024
12 checks passed

rustbot added this to the 1.77.0 milestone Jan 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement iterator specialization traits on more adapters #85528

Implement iterator specialization traits on more adapters #85528

the8472 commented May 20, 2021 •

edited

Loading

rust-highfive commented May 20, 2021

joshtriplett commented May 20, 2021

rust-timer commented May 20, 2021

bors commented May 20, 2021

bors commented May 20, 2021

rust-timer commented May 20, 2021

rust-timer commented May 21, 2021

the8472 commented May 21, 2021 •

edited

Loading

the8472 commented Jun 12, 2021

the8472 Jun 12, 2021

thomcc Aug 30, 2022

the8472 Sep 1, 2022

thomcc Sep 4, 2022

yaahc commented Jun 21, 2021

bors commented Jul 29, 2021

This comment has been minimized.

rfcbot commented Oct 8, 2023

the8472 commented Jan 10, 2024

dtolnay left a comment

dtolnay commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-log-analyzer commented Jan 21, 2024

the8472 commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Implement iterator specialization traits on more adapters #85528

Implement iterator specialization traits on more adapters #85528

Conversation

the8472 commented May 20, 2021 • edited Loading

rust-highfive commented May 20, 2021

joshtriplett commented May 20, 2021

rust-timer commented May 20, 2021

bors commented May 20, 2021

bors commented May 20, 2021

rust-timer commented May 20, 2021

rust-timer commented May 21, 2021

the8472 commented May 21, 2021 • edited Loading

the8472 commented Jun 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaahc commented Jun 21, 2021

bors commented Jul 29, 2021

This comment has been minimized.

rfcbot commented Oct 8, 2023

the8472 commented Jan 10, 2024

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-log-analyzer commented Jan 21, 2024

the8472 commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

rust-timer commented Jan 21, 2024

Overall result: ✅ improvements - no action needed

the8472 commented May 20, 2021 •

edited

Loading

the8472 commented May 21, 2021 •

edited

Loading